Skip to content

feat: Use slog for logging #1898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat: Use slog for logging #1898

wants to merge 4 commits into from

Conversation

arthurpitman
Copy link
Contributor

@arthurpitman arthurpitman commented May 16, 2025 •
edited
Loading

Why this PR?

What has changed?

  • Update some log messages to not use %q: as the default handler for slog places the value of each field, including message, inside "", using %q is not easy to read as it results in message="some \"thing\"". Using '', that is, writing '%s' is better as it results in message="some 'thing'".

    In this PR, we only update what is necessary to get the existing tests to pass; a future PR will address the log messages in general.

  • Remove internal/loggers package, remove go.uber.org/zap and github.com/go-logr/zapr dependencies.

How does it do it?

How is it tested?

How does it affect users?

@arthurpitman arthurpitman added the run-e2e-test Manually trigger the E2E tests for reviewed PRs label May 16, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented May 16, 2025 •
edited
Loading

E2E Test Results

    3 files  ±0    257 suites   - 6   20m 50s ⏱️ + 2m 11s
2 165 tests  - 9  2 157 ✅  - 14  2 💤 ±0  6 ❌ +5 
2 405 runs  ±0  2 397 ✅  -  5  2 💤 ±0  6 ❌ +5 

For more details on these failures, see this check.

Results for commit 3f70c4f. ± Comparison against base commit e0c6ee6.

This pull request removes 18 and adds 9 tests. Note that renamed tests count towards both. 
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestAllMethodsHaveFields
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestAllMethodsHaveFields/Debug_log_-_has_fields
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestAllMethodsHaveFields/Error_log_-_has_fields
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestAllMethodsHaveFields/Info_log_-_has_fields
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestAllMethodsHaveFields/Warn_log_-_has_fields
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestClone
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestEncodeEntry_IgnoresFieldsGivenViaArgs
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestEncodeEntry_UsesFieldsFromObjectEncoder
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestLoggerReturnsCustomLogLevell
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/loggers/zap ‑ TestLoggerReturnsDefaultLogLevel
…

github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestHandler_Handle
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestHandler_Handle/with_attributes
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestHandler_Handle/without_attributes
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestHandler_WithAttrs
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestHandler_WithGroup
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestTeeHandler_Enabled
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestTeeHandler_Enabled/all_enabled_if_both_have_level_debug
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestTeeHandler_Enabled/all_enabled_if_both_have_level_info
github.com/dynatrace/dynatrace-configuration-as-code/v2/internal/log ‑ TestTeeHandler_Enabled/all_enabled_if_one_has_level_debug,_other_info

♻️ This comment has been updated with latest results.

@arthurpitman arthurpitman added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels May 16, 2025
@arthurpitman arthurpitman added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels May 19, 2025
@arthurpitman arthurpitman added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels May 20, 2025
@arthurpitman
fix: Include missing coordinate value in log message
5780672
@arthurpitman
refactor: Refactor quoted strings in log messages ready for slog
43af8ca 
Log messages covered by tests should prefer `'%s'` over `%q` as these look better inside `""`.
@arthurpitman
test: TestSupportArchiveIsCreatedAsExpected ignores case when looki…
c908138 
…ng for debug content

Currently the logger writes the debug level as `debug` but slog writees `DEBUG`. Making it ignore case, makes it work in both cases
@arthurpitman arthurpitman added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels May 20, 2025
@arthurpitman arthurpitman added the run-e2e-test Manually trigger the E2E tests for reviewed PRs label May 20, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@arthurpitman arthurpitman added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
run-e2e-test Manually trigger the E2E tests for reviewed PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@arthurpitman